Skip to content

Remove YAML config for synology_dsm#42777

Closed
Quentame wants to merge 3 commits intohome-assistant:devfrom
hacf-fr:synology_dsm/remove_yaml
Closed

Remove YAML config for synology_dsm#42777
Quentame wants to merge 3 commits intohome-assistant:devfrom
hacf-fr:synology_dsm/remove_yaml

Conversation

@Quentame
Copy link
Copy Markdown
Member

@Quentame Quentame commented Nov 2, 2020 •
edited
Loading

Breaking change

Synology DSM has fully transitioned to configuration via UI. YAML configuration is no longer supported after being automatically imported for several releases.

Proposed change

Following ADR-0010: https://github.com/home-assistant/architecture/blob/master/adr/0010-integration-configuration.md

In draft because CONF_DISKS and CONF_VOLUMES need some love to be configured.
It was only possible to set them at import, so we should either:

  • remove them from the configuration (and disabled entries afterwards if wanted)
  • add a config in the options flow --> is a multi select available in the UI ?

What should I do ?

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Answer to #42539 (comment)

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@Quentame Quentame mentioned this pull request Nov 2, 2020
Merged
21 tasks
@Quentame Quentame force-pushed the synology_dsm/remove_yaml branch from dc5afc1 to ab62ab9 Compare November 2, 2020 23:35
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Nov 2, 2020

cv.multi_select works in config flows. homekit has an example.

@mib1185 mib1185 mentioned this pull request Nov 3, 2020
Closed
21 tasks
@mib1185 mib1185 mentioned this pull request Nov 14, 2020
Closed
@mib1185
Copy link
Copy Markdown
Member

mib1185 commented Nov 15, 2020

Hi @Quentame ,
I would like to contribute to this PR, but do not know how I can push to your hacf-fr:synology_dsm/remove_yaml or if there is another way to do so?

@Quentame
Copy link
Copy Markdown
Member Author

Quentame commented Nov 15, 2020

Hi !
I will give you rights on it tonight.

@Quentame
Copy link
Copy Markdown
Member Author

Quentame commented Nov 15, 2020

Added you to the repo.
You should now have access to it, with push rights.

TODO:

  • add disks and volumes to options config
  • use cv.multi_select to add select needed device (prefill with all)
  • if no change or full select, store nothing/None

While writing this, I am thinking that it would be easier to remove this config, and I think we should remove it.
This is why:

  • I don't think this is used anymore
  • Before config entry, fetching might be multiple per device, but not now
  • Before config entry, we couldn't remove/disable an entry via the UI, but not now
  • How should we handle when the user add a new device (disk or volume)? Add the new device automatically or wait for the user to add it ?
  • Same question when a device is removed (and will generate errors)
  • Removing this config will make the integration easier to maintain and avoid dev work.

What do you think @mib1185 ?

@mib1185
Copy link
Copy Markdown
Member

mib1185 commented Nov 15, 2020

Hi @Quentame ,

I have already pushed a commit, where I added an additional setup step to define, if user wants to limit/filter the disks and volumes to add, or not 😉
To address these two points:

  • How should we handle when the user add a new device (disk or volume)? Add the new device automatically or wait for the user to add it ?
  • Same question when a device is removed (and will generate errors)

I would suggest to implement something like "reconfigure is needed" as I already have seen on another integration 🤔

Quentame
Quentame commented Nov 15, 2020
View reviewed changes
Comment thread homeassistant/components/synology_dsm/config_flow.py Outdated
Comment on lines 257 to 262
Copy link
Copy Markdown
Member Author

@Quentame Quentame Nov 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devices should never be undefined
--> self.syno_info[CONF_...]

Copy link
Copy Markdown
Member

@mib1185 mib1185 Nov 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of an virtual DSM running on virtual machine manager, there are no disks available (see #42523), but yes for volumes - they never should be undefined ... I will change it

@Quentame
Copy link
Copy Markdown
Member Author

Quentame commented Nov 15, 2020

Yep, good work 👏🏼, but is this (future) work worth it ?

We should not code this for the fun of doing it but for the good of the integration.
I prefer to focus on fixing issues or adding new features to the end user. So that it will be use everyday and not only at config.

@mib1185 mib1185 force-pushed the synology_dsm/remove_yaml branch from 34de617 to 5bb9cf9 Compare November 15, 2020 20:48
@mib1185
Copy link
Copy Markdown
Member

mib1185 commented Nov 15, 2020

Yeah, I fully agree with you, that we should not code for the fun of doing it but for the good of the integration and we should focus on fixing issues or adding new features to the end user. 👍
But in this case, the work is nearly done and most of the logic to consider, if disks/volumes has been selected or not, is already in place from previous yaml configuration approach - examples:

core/homeassistant/components/synology_dsm/sensor.py

Line 45 in 149ba08

for volume in entry.data.get(CONF_VOLUMES, api.storage.volumes_ids):

core/homeassistant/components/synology_dsm/binary_sensor.py

Line 42 in 149ba08

for disk in entry.data.get(CONF_DISKS, api.storage.disks_ids):

@mib1185 mib1185 force-pushed the synology_dsm/remove_yaml branch from 5bb9cf9 to 9c3f426 Compare November 15, 2020 20:59
@Quentame Quentame force-pushed the synology_dsm/remove_yaml branch from 9c3f426 to 506d537 Compare November 17, 2020 23:01
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 26, 2021

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions Bot added the stale label Jan 26, 2021
@frenck
Copy link
Copy Markdown
Member

frenck commented Jan 28, 2021

This PR has been in a draft and running stale without activity since November.
I'll go ahead and close this PR for now. Feel free to re-open the PR again when ready to start working on it. Contributions are always appreciated 👍

@frenck frenck closed this Jan 28, 2021
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mib1185 mib1185 mib1185 left review comments

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

breaking-change cla-signed has-tests integration: synology_dsm Synology DSM small-pr PRs with less than 30 lines. stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Quentame @bdraco @mib1185 @frenck @MartinHjelmare @homeassistant